Support multiple player profiles with switching by carochacs · Pull Request #3 · get-flashbacks/feedBack

carochacs · 2026-07-31T21:19:29Z

What

Adds "who's playing" style multi-profile support so more than one person can share a feedBack install without mixing progress. Player identity, XP/level/streak, per-song practice stats, favorites, and career progression (paths/challenges/quests/wallet/shop) — plus the achievements/Feats plugin's own local store — are now scoped per-profile instead of one implicit device-wide profile.

New profiles table replacing the old CHECK(id=1) singleton profile row; a device-local active_profile pointer (no auth system exists to hang a session off, so switching is a full-page reload rather than per-request).
New endpoints: GET /api/profiles (list), POST /api/profiles (create), POST /api/profiles/{id}/activate (switch), DELETE /api/profiles/{id} (delete a profile and everything scoped to it — refuses to delete the active or the only remaining profile).
Existing endpoints (/api/profile, /api/stats, /api/progression, /api/shop, …) are unchanged in shape — they now implicitly operate on whichever profile is active, so no caller elsewhere in the app (including the career plugin, which reads meta_db directly) needed to change.
Achievements plugin's own sqlite tables (unlocks, counters, comp_ledger) get the same profile_id treatment.
Frontend: a "Switch profile" button on the Profile screen opens a picker to switch or create profiles; switching does a full page reload.
Upgrading installs migrate their existing single profile's data to profile 1 automatically (idempotent schema migration, verified against a hand-built pre-migration DB), with zero behavior change until a second profile is added.

Known v1 scope boundary, called out explicitly rather than silently dropped: playlists/collections and saved practice loops remain device-wide (not yet per-profile) — a documented follow-up.

feedpak surface

This PR does not change how the app reads/writes feedpaks (manifest keys, pack files, folder layout) — this only touches the app's own SQLite metadata store (web_library.db, achievements.db), not the sloppak/feedpak chart format.

Checklist

CHANGELOG.md [Unreleased] updated (user-visible changes)
Tests added/updated for new behaviour (profile CRUD, cross-profile isolation for identity/XP/stats/wallet/shop/achievements, upgrade-migration smoke test)
Commits are DCO signed off (git commit -s)

Validation

Full test suite: 2714 passed, 4 skipped (unrelated missing optional deps), 0 failed.

Generated by Claude Code

carochacs

Reviewed the full diff (metadata_db.py schema/migration, lib/routers/profile.py, plugins/achievements/routes.py, static/v3/profile.js, tests).

No TODOs/NotImplementedError; profile scoping is consistently applied across favorites, XP, streaks, progression, wallet/shop via idempotent migration helpers that read live schema rather than hardcoding columns.
delete_profile correctly refuses to delete the active or last remaining profile, with lock-guarded cleanup across all associated tables.
Follows repo conventions: flat imports, threading.Lock around writes, parameterized SQL throughout, vanilla JS, no new frontend frameworks.
No auth-bypass or path-traversal concerns (this is explicitly a non-auth "who's playing" picker, per the PR). display_name is escaped client-side before insertion into innerHTML — no stored-XSS surface. All SQL uses parameterized placeholders.
Minor non-blocking note: activate_profile's existence check + insert/update inside _lock isn't strictly atomic against a concurrent delete_profile of the same id — low risk given the single-device usage model, and no unsafe SQL either way.

No blocking issues found — clear to merge once out of draft. (Could not submit as an "Approve" — GitHub blocks self-approval since this PR and the reviewing account share the same author.)

Generated by Claude Code

coderabbitai · 2026-08-10T11:43:38Z

Important

Approval pending

CodeRabbit has no unresolved comments, but it has not reviewed the latest commit.

Use the checkbox below to review the latest commit. CodeRabbit will approve the changes if it finds no blocking issues.

🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

pullfrog · 2026-08-10T11:44:05Z

origin/main is ec1157a, and 03e1c1d is reported as not an ancestor of it locally — yet it's a main-merged PR. The local clone is likely shallow, which distorts rev-list. CI (full-depth checkout) sees the true range as just {5d9dcc5, 566e75d} — only 5d9dcc5 is genuinely flagged. Let me confirm shalowness before rewriting.

Task list (1/6 completed)

Checkout PR branch and fetch check suite logs
Verify the failure was introduced by this PR (not pre-existing)
Diagnose the CI failure
Fix the issue and verify locally with the exact CI command
Commit and push changes to the existing PR
Report progress with diagnosis and fix summary

^{｜ View workflow run ｜ via Pullfrog ｜ Using Big Pickle (free) ｜ 𝕏}

deepsource-io · 2026-08-10T11:44:26Z

DeepSource Code Review

We reviewed changes in bf80de3...5c267f1 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.

PR Report Card

Overall Grade Focus Area: Security	Security Reliability Complexity Hygiene

Feedback

Nullable state leaking through the call graph

The type issues all connect back to None flowing where an int, dict, or tuple is expected, and then being indexed or unpacked (get_collection, tunings, plugins, song).
It’s one underlying question: which things are truly optional vs guaranteed? Tightening that contract once would simplify the whole chain.

Database boundary assumptions

Both the SQL injection warnings and the next()-without-guard issues live around metadata DB access and query handling.
Worth thinking of the DB layer as untrusted in both directions: sanitize what you send, and treat iteration / lookups as potentially empty or failing.

Code Review Summary

Analyzer	Updated (UTC)	Details
Docker	Aug 16, 2026 3:12p.m.	Review ↗
JavaScript	Aug 16, 2026 3:12p.m.	Review ↗
Python	Aug 16, 2026 3:12p.m.	Review ↗
Shell	Aug 16, 2026 3:12p.m.	Review ↗

Important

AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.

pullfrog · 2026-08-15T04:38:45Z

Fixed the failed core-commit-labeling check on PR #3 and pushed the corrected history.

Diagnosis. The only failing job (core-commit-labeling, from .github/workflows/fork-audit.yml) flagged commit 5d9dcc5 (feat(profile): support multiple player profiles with switching): it touches core paths (lib/metadata_db.py, lib/routers/profile.py, static/v3/profile.js) but its subject lacks the required core:/hook:/sync:/fix: prefix (docs/fork-maintenance.md Rule 2). No code or test failure — the PR's own content is fine.

Fix. A follow-up commit cannot satisfy the check (it iterates every commit in BASE..HEAD), so the offending commit's subject had to be rewritten in place. I rebuilt the branch's history on the shallow clone:

Relabeled 5d9dcc5's subject to core(profile): support multiple player profiles with switching (via amend, preserving author, date, body, and tree 6549b54d).
Recreated the 566e75d merge commit with the same tree (081536bd) and parents, preserving its message.
Recreated the tip 0e70e76 with its tree (00e3525a) unchanged.
Force-pushed to claude/feedback-profile-swap-mul3fs (now at 7349362).

Verification. Each rewritten commit's tree hash matches the original, so the PR's content is byte-identical. The merge remains a tree no-op vs its first parent (exempt from the check), the tip touches only plugins/achievements/routes.py (excluded), and the relabeled subject matches the workflow's regex — the check now passes. Re-running CI on the new head should go green.

Task list (7/7 completed)

Checkout PR Support multiple player profiles with switching #3 branch
Fetch check suite logs to diagnose failure
Verify failure was introduced by this PR
Diagnose and fix the issue
Verify fix by re-running CI command locally
Commit and push changes to PR Support multiple player profiles with switching #3
Report progress

^{｜ View workflow run ｜ via Pullfrog ｜ Using Big Pickle (free) ｜ 𝕏}

Player identity, XP/level/streak, per-song practice stats, favorites, career progression (paths/challenges/quests/wallet/shop), and achievements/Feats were all keyed to an implicit single device-wide profile (several tables even had a CHECK(id=1) constraint baked in). Households sharing one feedBack instance had no way to keep separate progress, so this adds real multi-profile support: a `profiles` table, a device-local active-profile pointer, and profile_id scoping threaded through every one of those subsystems (including the achievements plugin's own sqlite tables). Existing installs migrate their single profile's data to profile 1 automatically, with zero behavior change until a second profile is added. New endpoints: GET/POST /api/profiles, POST /api/profiles/{id}/activate, DELETE /api/profiles/{id}. Existing endpoints are unchanged in shape — they now implicitly read/write whichever profile is active. A "Switch profile" button on the Profile screen opens a picker to switch or add profiles; switching reloads the page since there's no session/auth system to scope a switch to more narrowly. Playlists/collections and saved practice loops remain device-wide for now (documented as a known v1 gap, not silently dropped).

…le-swap-mul3fs # Conflicts: # CHANGELOG.md

…hievements plugin

pullfrog

Important

The new 0e70e76 commit (sha1→sha256 in the achievements plugin's _chart_key) breaks an existing test and silently resets persisted per-chart play counters on upgrade. tests/plugins/achievements/test_routes.py:82 pins _chart_key to the sha1 digest and now fails (verified locally — this makes the PR's pytest check red), and because chart_plays:* keys are persisted in the counters table, swapping the hash orphans every previously accumulated Encore play count on an upgrading install — with no real security gain, since the digest is truncated to 16 hex chars and the input isn't attacker-controlled. Details inline on plugins/achievements/routes.py:311.

Reviewed changes

0e70e76 — Swapped the achievements plugin's per-chart counter key hash from sha1 to sha256 (docstring + _chart_key return line in plugins/achievements/routes.py), with no accompanying test update or counter migration.
566e75d — Merged origin/main into the branch; the range-diff contains no PR-specific code from this merge.

^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

…ash to sha1 - create_profile()/activate_profile() now read the row back under the same lock instead of after releasing it, closing the (narrow) window where a concurrent delete_profile() could remove the row first and raise an unhandled StopIteration. - _chart_key() reverts sha256 back to sha1: the digest is truncated to 16 hex chars either way, so the truncation defines the collision space regardless of algorithm, and the input isn't attacker-controlled. The sha256 swap broke test_chart_key_is_stable_not_builtin_hash (still pinned to sha1) and would have silently reset every upgrading install's persisted Encore Feat counters.

deepsource-io · 2026-08-16T15:14:49Z

+    input isn't attacker-controlled. Swapping to sha256 would re-key every
+    counter already persisted under the sha1 form, silently resetting every
+    upgrading install's Encore Feat progress for no real security gain."""
    return "chart_plays:" + hashlib.sha1(str(chart).encode("utf-8")).hexdigest()[:16]


Use of insecure hashlib.sha1 hash function

D2, MD4, MD5, SHA1 signature algorithms are known to be vulnerable to collision attacks. Attackers can exploit this to generate another certificate with the same digital signature, allowing them to masquerade as the affected service.

pullfrog

✅ No new issues found.

Reviewed changes

Re-reviewed the incremental delta since the prior Pullfrog review (commit 5c267f1 on top of 7349362), against the full PR diff.

Reverted _chart_key to sha1 in the achievements plugin — resolves the prior [!IMPORTANT] finding: persisted chart_plays:<sha1> counter rows stay readable on upgrade (no silent Encore Feat per-chart play-count reset) and the pinned test_chart_key_is_stable_not_builtin_hash test passes again. The expanded docstring records why sha1 is deliberate — the 16-hex truncation defines the collision space either way and the input isn't attacker-controlled.
Closed the profile create/activate read-back race — create_profile and activate_profile now read back under self._lock instead of after an unlocked list_profiles() call, so a concurrent delete_profile() can't remove the just-written row before the read-back. list_profiles() is lock-free (reads don't take the non-reentrant lock), so there's no deadlock, and the read-back always finds the row.

Verified locally: the achievements + profile + progression suites pass (142 tests), and the full pytest run is green through completion. No new issues in the delta.

^{｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

pullfrog · 2026-09-04T21:30:18Z

Fixed the core-commit-labeling CI failure on PR #3.

Diagnosis: The failing check suite (91933004708) is fork-audit → core-commit-labeling. It failed on the PR head commit a0a2960 ("Merge remote-tracking branch 'origin/main' into profile-merge"): a merge commit that resolved a conflict in plugins/achievements/routes.py and, relative to its first parent, also changed the core file lib/metadata_db.py — so it needs a core:/hook:/sync:/fix: subject prefix per docs/fork-maintenance.md Rule 2 (regex in .github/workflows/fork-audit.yml:43). It was the only offending commit in the base..head range.

Fix: Rewrote the merge commit's subject as sync: … (the policy prefix for merges that bring base/upstream changes in), preserving author, both parents (5c267f1, be5de31), the full body (# Conflicts: note), and the tree (^{tree} unchanged, verified). Since it was the PR tip, git commit --amend -F did the relabel without touching any other commit, then force-pushed the branch: remote head is now 6d1129c.

Verification: The merged tree's lib/metadata_db.py still carries the full profile feature (profiles/active_profile tables, get_active_profile_id, profile-scoped queries), so this was purely a labeling issue, not a content regression. Re-running the CI check step against the new head passes the subject regex for every core-touching commit in the range.

Task list (7/7 completed)

Checkout PR branch via pullfrog_checkout_pr
Fetch check suite logs via pullfrog_get_check_suite_logs
Verify the failure was introduced by this PR
Diagnose and fix the issue
Verify fix by re-running the exact CI command
Commit and push changes to the existing PR
Report progress with diagnosis and fix summary

^{｜ View workflow run ｜ via Pullfrog ｜ Using Big Pickle (free) ｜ 𝕏}

# Conflicts: # plugins/achievements/routes.py

pullfrog

✅ No new issues found.

Reviewed changes

Incremental re-review of the delta since the prior Pullfrog review (commit 5c267f1): the merge of origin/main into the branch (a0a2960) and the commits it brings, checked against the full PR diff.

Merged origin/main — pulls in the fork's deepsource-autofix sha256 change (9b33620) plus unrelated main-side commits (docs, CSP worker-src, demo-mode lyrics block). The merge is clean: vs 5c267f1 it touches only lib/metadata_db.py and plugins/achievements/routes.py, no branch profile-scoping work was dropped, and no conflict-resolution artifacts remain.
_chart_key now chart_plays:v2:<sha256[:16]> — supersedes the 5c267f1 sha1 revert that closed the prior [!IMPORTANT] thread. The swap still orphans pre-existing chart_plays:<sha1> rows, but it is no longer silent: the versioned v2: prefix makes the generation change explicit, the pinned test_chart_key_is_stable_not_builtin_hash test was updated to the new form, the chart_encore_max aggregate survives the reset, and _read_counters' NOT LIKE 'chart_plays:%' filter still excludes the rows. This matches fork main exactly, so the branch is back in lock-step with its merge target.
_migrate_enrichment_hash_sha256() added to the startup migration chain — re-stamps legacy sha1 song_enrichment.content_hash rows in place (detected by 40-char digest length), keeping settled matched/review/failed rows from being re-queued for enrichment on upgrade. It runs alongside the PR's profile migrations and is independent of them.

Verified at head: the targeted suites pass (73 tests across achievements, profile, progression, and enrichment), and the 5c267f1 → a0a2960 delta is confined to the two files above.

^{｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

…d-back create_profile/activate_profile read the row back under the same lock that serializes against delete_profile, so the generator is not expected to be empty in practice -- but an unwrapped next() with no default would surface as a bare, confusing StopIteration if that invariant were ever broken by a future change. Wrap both in try/except and raise a clear RuntimeError instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011EJ2yKGrVxKi5CR5dWoND3

pullfrog · 2026-09-04T22:57:59Z

Pullfrog stalled — likely cause: rate limited

The agent stopped emitting events for 120s and was killed by the activity-timeout watchdog. 88 events were processed before the failure.

Recent agent stderr

timestamp=2026-09-04T22:58:53.697Z level=ERROR run=d78d08d0 message="stream error" providerID=opencode modelID=big-pickle session.id=ses_f915a90c4ffeSbQOryBteI8saG small=false agent=build mode=primary error.error="AI_APICallError: Rate limit exceeded. Please try again later."

^{｜ Rerun failed job ➔ ｜ View workflow run ｜ via Pullfrog ｜ Using Big Pickle (free) ｜ 𝕏}

pullfrog

✅ No new issues found.

Reviewed changes

Incremental re-review of the delta since the prior Pullfrog review (6d1129c → 7c1039a), checked against the full PR diff.

Fail loudly on the profile create/activate read-back — create_profile and activate_profile now wrap their next() read-back in try/except StopIteration and re-raise a descriptive RuntimeError (profile N vanished immediately after creation/activation, chained from None), replacing what was a bare generator StopIteration 500 with an actionable one. This also retires the production-code half of the earlier DeepSource "next() without try/except" note on lib/metadata_db.py (the two test-side instances remain, but only run after a successful response, so they can't trip).

Assessment: create_profile's read-back sits under self._lock, so a concurrent delete_profile cannot interleave there — the guard is defensive. activate_profile retains the previously documented existence-check window, where a stale activate racing a concurrent delete now fails with a clear message instead of an obscure traceback. Verified the profile + progression + achievements suites pass (54 tests).

^{｜ View workflow run ｜ Using Big Pickle (free) ｜ 𝕏}

carochacs · 2026-09-04T22:58:11Z

ci / tailwind-fresh is failing on this PR, but it's not this PR's diff — this branch's static/tailwind.min.css is byte-identical to main's (last touched by 270cb39, unrelated to profiles), so the same failure reproduces identically against a clean main checkout. It's toolchain drift (the script's own comment calls this out: byte-stable rebuilds need the exact resolved dependency tree, and transitive deps like postcss/cssnano can drift even with tailwindcss pinned).

Not pushing a fix here — regenerating the CSS is an unrelated file with no connection to profile-swap, and belongs in its own chore: PR rather than widening this one. The fix, for whoever picks it up:

npm ci && bash scripts/build-tailwind.sh

Only a 1-line diff in the header comment / minified output — git diff --stat shows static/tailwind.min.css | 2 +-.
Claude-Session: https://claude.ai/code/session_011EJ2yKGrVxKi5CR5dWoND3

Generated by Claude Code

carochacs mentioned this pull request Jul 31, 2026

TASK: SOTA Research Initiative — Priority Matrix & Repository Impact Analysis #14

Open

carochacs commented Jul 31, 2026

View reviewed changes

carochacs added the enhancement New feature or request label Jul 31, 2026

carochacs self-assigned this Aug 3, 2026

deepsource-io Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread lib/metadata_db.py Outdated

Comment thread lib/metadata_db.py Outdated

Comment thread lib/metadata_db.py

Comment thread tests/test_profile_api.py

Comment thread tests/test_profile_api.py

deepsource-io Bot reviewed Aug 10, 2026

View reviewed changes

deepsource-io Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread static/v3/profile.js

claude and others added 3 commits August 15, 2026 04:41

Merge remote-tracking branch 'origin/main' into claude/feedback-profi…

efae603

…le-swap-mul3fs # Conflicts: # CHANGELOG.md

fix: replace insecure sha1 with sha256 for chart key generation in ac…

7349362

…hievements plugin

pullfrog Bot force-pushed the claude/feedback-profile-swap-mul3fs branch from 0e70e76 to 7349362 Compare August 15, 2026 04:42

pullfrog Bot reviewed Aug 15, 2026

View reviewed changes

Comment thread plugins/achievements/routes.py Outdated

deepsource-io Bot reviewed Aug 16, 2026

View reviewed changes

pullfrog Bot reviewed Aug 16, 2026

View reviewed changes

sync: Merge remote-tracking branch 'origin/main' into profile-merge

6d1129c

# Conflicts: # plugins/achievements/routes.py

pullfrog Bot force-pushed the claude/feedback-profile-swap-mul3fs branch from a0a2960 to 6d1129c Compare September 4, 2026 21:34

pullfrog Bot reviewed Sep 4, 2026

View reviewed changes

Uh oh!

Conversation

carochacs commented Jul 31, 2026

What

feedpak surface

Checklist

Validation

Uh oh!

carochacs left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Approval pending

Uh oh!

pullfrog Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deepsource-io Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepSource Code Review

PR Report Card

Feedback

Code Review Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pullfrog Bot commented Aug 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

deepsource-io Bot Aug 16, 2026

Choose a reason for hiding this comment

Use of insecure hashlib.sha1 hash function

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

carochacs commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

coderabbitai Bot commented Aug 10, 2026 •

edited

Loading

pullfrog Bot commented Aug 10, 2026 •

edited

Loading

deepsource-io Bot commented Aug 10, 2026 •

edited

Loading

pullfrog Bot commented Aug 15, 2026 •

edited

Loading

pullfrog Bot commented Sep 4, 2026 •

edited

Loading

pullfrog Bot commented Sep 4, 2026 •

edited

Loading